Skip to content

Recycle temporary expression scopes on success and failure - #39

Open
284km wants to merge 1 commit into
mainfrom
perf/reuse-scratch-scopes
Open

284km wants to merge 1 commit into
mainfrom
perf/reuse-scratch-scopes

Conversation

@284km

@284km 284km commented Nov 7, 2026 •

Copy link
Copy Markdown
Owner

Temporary scopes for comparisons and attribute/index updates inherited the surrounding frame's capture marker. A closure holding the parent therefore prevented unrelated temporary copies from returning to the frame pool. Exceptions could also bypass cleanup entirely. Repeated expressions eventually exhausted the CI memory budget.

Clear inherited capture identity on detached copies, retain and register genuine captures, and use a shared cleanup path on both normal return and failure. Add a workload that checks values, closure lifetime and original exceptions, plus a macOS/Linux CI check that verifies frame allocations remain bounded as the iteration count grows.

Validation:

  • At 100 and 10,000 iterations, fresh frame counts fell from 602 / 60,002 to 3 / 3. The regression passes with hash indexing ON and OFF.
  • The unchanged TestHashOnly#test_broken_hash_value exceeded the 2 GiB limit on main e45622a; the fix passes both assertions in 31.9 seconds with 269,344 KiB peak process-family RSS.
  • All 522 corpus programs match Ruby with hash indexing ON and OFF. The new bounded-allocation check also passes on Linux at O2.
  • The full Hash file now completes: 306 tests, 1,902 assertions, 0 failures, 1 existing timeout error and 8 skips, at 714,544 KiB peak RSS. The isolated test_replace_memory_leak timeout also reproduces on main; this is not a claim of full Hash compatibility.
  • The CI Hash shard also completes all 306 tests (297 MATCH, 1 existing timeout, 8 shared skips), at 678,896 KiB peak process-family RSS. Other CI shards are still running or queued. This PR remains draft pending the final checks.

@284km 284km left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed d325094 against e45622a. I found no actionable issues in this change.

Clearing the detached copy's inherited capture marker is consistent with its lack of a parent link. Routing cleanup through blk_env_put also preserves genuinely captured frames and registers them for later collection. All three scratch-scope call sites now release their temporary on success and failure.

Validation:

  • Built the exact PR head at O2 with pinned Mere compiler 991f299b.
  • Ran unittest/scratch_scope_check.py: fresh frame counts were 3 / 3 at 100 / 10,000 iterations, with hash indexing ON and OFF.
  • Additional probes matched Ruby 4.0.9 for assignment results, short-circuiting, getter/setter/comparison exceptions (including exception identity and $!), retained bindings and procs after frame reuse and GC, and nonlocal return, break, and throw. These passed with indexing ON/OFF, both normally and with MERE_RUBY_GC_STRESS=100.
  • Checked CI run 42368677643 for this exact head: 522/522 corpus programs matched Ruby; the new allocation regression passed on macOS and Linux. Some comprehensive test shards are still queued or running, so this is not a claim that the entire suite has completed.

Posting as COMMENT because the authenticated reviewer is also the PR author.

Comment thread m_state.mere Resolved
Comment thread unittest/scratch_scope_check.py Resolved
@284km

284km commented Nov 7, 2026

Copy link
Copy Markdown
Owner Author

Thanks for checking exception identity, nonlocal control flow, and retained bindings/procs under GC stress in addition to the allocation regression. No changes are needed for this review. The CI Hash shard has now completed all 306 tests below the memory limit (678,896 KiB peak RSS); the remaining single timeout reproduces on the baseline. I am waiting for the remaining shards and final report before marking this ready.

@284km

284km commented Nov 7, 2026

Copy link
Copy Markdown
Owner Author

Final validation for d325094: CI run 42368677643 passed all 29 jobs. The report collected all 758 files and 8/8 shards with zero missing files, infrastructure errors, or WALL_LIMIT files. The Hash target completed all 306 tests: 297 MATCH, one pre-existing timeout, and eight reference skips. The macOS/Linux allocation regression, both hash-index modes, and review checks of captured bindings/procs and exception/nonlocal control flow all passed.

The report retains 17 MEMORY_LIMIT files and 161 incomplete/zero-test files overall. I investigated the Module limit, which was absent from the earlier 278a46c report: the exact pre-change e45622a binary and this candidate both hit 2 GiB at TestModule#test_complemented_method_entry_memory_leak locally (40.173 s / 2,122,320 KiB and 42.023 s / 2,123,632 KiB respectively). It is reproducible without this change. Remaining compatibility/resource failures are not being hidden or counted as passing tests.

The review has no actionable findings and its threads are resolved. Marking this PR ready. #38 depends on this branch and is still being validated separately.

@284km
284km marked this pull request as ready for review November 7, 2026 14:03

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant